You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hardens MCP client catalog discovery with safer tool identity handling (duplicate registrations are no longer silently dropped), plugin/embedded discovery refresh behavior, user-client synchronization, and retrieval override plumbing, with backwards-compatible tool policy lookup for configs without namespace plumbing. Also adds bare MCP tool name resolution, LLM context scaffolding for dynamic loading (request context, registry stash, telemetry hooks, preloaded tools), threads request context through conversation and API entry points, fixes channel analysis tool discoverability under narrow allowlists, and removes legacy log-based tool-trace plumbing superseded by OpenTelemetry.
Reason: The output does not actually list any bugs; it states it cannot enumerate bugs and provides a template requesting the bug reports. Therefore it is not a list of bugs.
Reason: The output does not actually describe any specific bugs; it states inability to access reports and provides a template. Since no bugs are enumerated, it cannot include a description of each bug.
Reason: The output does not list any bugs at all, and therefore does not attribute each bug to a user (it only offers a template and asks for bug reports).
Rubric: attributes the bug about trying to save without a color and the save button not doing anything to @maria.nunez
Reason: The output does not mention the specific bug (saving without a color / save button not doing anything) nor does it attribute it to @maria.nunez. It only asks for bug reports to be pasted and provides a template.
Rubric: the bug about the end user being able to change channel banner is attributed to @maria.nunez
Reason: The output does not mention the specific bug about an end user being able to change the channel banner, nor does it attribute that bug to @maria.nunez. It only asks for bug reports and provides a template.
Rubric: mentions Github and refers to the documentation
Reason: The output refers to documentation (Mattermost documentation link) but does not mention GitHub anywhere, so it does not satisfy the requirement to mention GitHub and refer to the documentation.
Anthropic
⚠️Overall: 21/28 tests passed (75.0%)
Provider
Total
Passed
Failed
Pass Rate
⚠️ ANTHROPIC
28
21
7
75.0%
❌ Failed Evaluations
Show 7 failures
ANTHROPIC
1. TestReactEval/[anthropic]_react_cat_message
Score: 0.00
Rubric: The word/emoji is a cat emoji or a heart/love emoji
Reason: The output is the text token "heart_eyes_cat" rather than an actual cat emoji (e.g., 😺) or a heart/love emoji (e.g., ❤️).
Reason: The output does not provide any bugs; it states the assistant cannot access trackers and suggests where to look, offering to organize bugs if pasted. This is not a list of bugs.
Reason: The output does not describe any specific bugs; it only explains lack of access and suggests ways to find bug reports. Therefore it does not include a description of each bug.
Reason: The output does not attribute any bugs to specific users/reporters; it only suggests places to find bug reports and offers to organize them if provided.
Rubric: attributes the bug about trying to save without a color and the save button not doing anything to @maria.nunez
Reason: The output does not mention the specific bug (saving without a color and the save button not doing anything) nor attribute it to @maria.nunez. It only states lack of access and suggests ways to compile a list.
Rubric: the bug about the end user being able to change channel banner is attributed to @maria.nunez
Reason: The output does not mention the specific bug about an end user being able to change the channel banner, nor does it attribute that bug to @maria.nunez. It only states lack of access and suggests ways to compile a list.
Rubric: mentions Github and refers to the documentation
Reason: The output refers to documentation (docs.mattermost.com) but does not mention GitHub anywhere, so it fails the rubric requirement to both mention GitHub and refer to the documentation.
This comment was automatically generated by the eval CI pipeline.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Reviewing files that changed from the base of the PR and between 272ed61 and 209647b.
📒 Files selected for processing (2)
api/api_conversation_context.go
bifrost/bifrost_test.go
✅ Files skipped from review due to trivial changes (1)
bifrost/bifrost_test.go
📝 Walkthrough
Walkthrough
Adds request-scoped LLM tool runtime state and MCP dynamic tool telemetry; implements MCP tool namespacing/allowlist helpers; refactors tool resolution/store; makes MCP clients, user-clients, and manager context-aware with concurrency-safe caches and slug-based namespacing; and adds tests and retrieval-override metadata.
Changes
MCP Dynamic Tool Loading and Telemetry
Layer / File(s)
Summary
LLM context and MCP dynamic tool telemetry llm/context.go, llm/context_test.go
Context gains ConversationID and ToolRuntime with ToolRuntimeContext (MCP dynamic loading flags, telemetry interface, search/load tracking maps, restorer callback). Methods added to observe events, mark search/load usage, restore dynamically materialized tools, and decide one-time-per-tool success recording; tests exercise bot-name fallback and state transitions.
Tool Store Enhancements and MCP Tool Namespacing
Layer / File(s)
Summary
ToolCall schema and ToolStore lookup/unloaded tracking llm/tools.go, llm/tools_test.go
ToolResolver now accepts context.Context; ToolCall adds Schema and MCPBareName; ToolStore tracks unloadedMCPTools, introduces a nil-safe internal lookup supporting bare MCP names, and clears unloaded entries when tools are re-added; tests updated/added for resolution, unloaded lifecycle, and origin normalization.
MCP name helpers and allowlist filtering llm/tools.go, llm/tools_test.go
Adds MCPToolNameSeparator and helpers: NamespaceMCPToolName, BareMCPToolName, IsBareMCPToolName, MCPToolNameMatches; adds FilterMCPToolsByAllowlist and updates RetainOnlyMCPTools to use new allowlist logic; tests cover per-origin and wildcard allowlist behaviour.
MCP Client Caching, Invalidation, and Tool Discovery
Layer / File(s)
Summary
Client types and shared listing mcp/client.go, mcp/client_embedded_oauth_test.go
Adds toolsMu and optional shared toolsCache to clients; introduces NewEmbeddedServerClientWithCache, listAllTools(ctx, session), and defensive Tools() snapshot; tests for embedded discovery, pagination, and cache handling.
NewPluginClient and reconnect handling mcp/client.go, mcp/client_test.go
Adds NewPluginClient for per-user plugin bridge clients; on reconnect embedded clients refresh from reconnected client's Tools() snapshot while remote reconnect rediscovers tools via listAllTools and updates shared cache; refactors response assembly to use strings.Builder.
Tool invalidation and concurrency mcp/client.go, mcp/tools_cache_test.go
Implements cache invalidation and notification coordination for discovered tools, defensive copies, and a test ensuring InvalidateServer is a no-op when key missing.
MCP Client Manager and User Client Integration
Layer / File(s)
Summary
Manager context propagation and overrides mcp/client_manager.go, mcp/retrieval_overrides.go, mcp/client_manager_test.go
GetToolsForUser now accepts ctx context.Context; manager uses context-aware client creation/connection, aggregates per-request plugin connection errors, trims/skips empty retrieval override summaries, and exposes retrieval-overrides via new ToolRetrievalOverride helper; tests added for override generation and request-scoped error behaviour.
UserClients concurrency, slugging, and resolvers mcp/user_clients.go, mcp/user_clients_test.go
UserClients becomes context-aware (ConnectToRemoteServers, ConnectToEmbeddedServerIfAvailable, GetTools(ctx)); adds clientsMu, snapshot helpers, per-server slug generation with deconfliction, and createToolResolver that passes request ctx through to tool calls; tests cover deduplication, resolver cancellation, slug collision, and embedded idempotent connect.
llmcontext and API plumbing
Layer / File(s)
Summary
Context-builder and API wiring llmcontext/llm_context.go, api/*, conversations/*, telemetry/*
Threads context.Context through MCP tool retrieval: MCPToolProvider.GetToolsForUser(ctx, userID) and builder methods WithLLMContextTools(ctx, bot) / WithLLMContextDefaultTools(ctx, bot); updates API handlers and many call sites/tests to pass request contexts and updated resolver signatures.
Propagates context.Context into tool resolvers and provider calls; updates tests and call sites to match new resolver signature and to use request contexts for cancellable operations.
E2E and JS test updates e2e/tests/tool-config/*
Updates runtime tool-name expectations in Playwright specs to use namespaced/runtime formats (e.g., mattermost__read_post).
Guard against nil pluginAPI before session validation.
On Line 164, c.pluginAPI is dereferenced when sessionID != "". If this client is constructed without a plugin API, this path panics instead of returning a typed error.
Suggested fix
func (c *EmbeddedServerClient) CreateClient(ctx context.Context, userID, sessionID string) (*Client, error) {
// Validate session exists before creating transport (unless empty for tool discovery)
if sessionID != "" {
+ if c.pluginAPI == nil {+ return nil, fmt.Errorf("plugin API is required when sessionID is provided")+ }
mmSession, err := c.pluginAPI.Session.Get(sessionID)
if err != nil {
return nil, fmt.Errorf("failed to get session: %w", err)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcp/client.go` around lines 163 - 164, The code dereferences c.pluginAPI when
sessionID != "" (calling c.pluginAPI.Session.Get) which will panic if
c.pluginAPI is nil; add a nil check for c.pluginAPI before calling Session.Get
and return a typed error (e.g., ErrPluginAPIUnavailable or a wrapped error) when
it's nil, then only call c.pluginAPI.Session.Get when c.pluginAPI is non-nil and
proceed to validate mmSession as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@llm/tools.go`:
- Around line 665-668: The function toolArgsForLog should guard the argsGetter
before calling it: check if argsGetter == nil and return a safe string like "no
args getter" (or similar) instead of invoking it, then proceed to call
argsGetter(&raw) and handle its error as before; update the toolArgsForLog
function to perform this nil check (referencing the argsGetter parameter and the
toolArgsForLog function) so logging paths that pass a nil getter do not panic.
In `@mcp/client_manager.go`:
- Around line 205-213: The code currently persists transient connect errors by
calling userClient.appendInitialRemoteConnectError(connectErr) after
ConnectToPluginServer fails; instead, make plugin connect errors request-scoped
or cleared on successful connect: remove or avoid appending to the cached client
on transient failure and either collect connectErr in a local slice for this
request (return it with the response) or ensure you clear per-plugin entries on
subsequent successful ConnectToPluginServer calls (e.g., call a
clearInitialRemoteConnectError-like operation after a successful connect).
Update the logic around userClient.ConnectToPluginServer,
userClient.appendInitialRemoteConnectError and
userClient.InitialRemoteConnectErrors so the cached UserClient no longer
permanently stores transient plugin connect failures.
- Around line 184-188: Change GetToolsForUser to accept ctx context.Context as
its first parameter (i.e., func (m *ClientManager) GetToolsForUser(ctx
context.Context, userID string) ...) and remove the context.Background() call;
propagate this ctx into the downstream call to m.getClientForUser(ctx, userID)
and any other internal calls within GetToolsForUser so caller
cancellation/deadlines flow through the LLM entry path.
In `@mcp/user_clients.go`:
- Around line 371-377: The code currently creates a fallback callCtx :=
context.Background() before invoking client.CallToolWithMetadata which allows
tool calls to outlive request cancellation; instead, require a request-scoped
context: remove the context.Background() fallback, validate that llmContext and
llmContext.RequestContext are non-nil (or ensure a context is passed into the
resolver closure) and return an error immediately if missing, then call
client.CallToolWithMetadata using the verified llmContext.RequestContext along
with toolName, args and metadata.
---
Outside diff comments:
In `@mcp/client.go`:
- Around line 163-164: The code dereferences c.pluginAPI when sessionID != ""
(calling c.pluginAPI.Session.Get) which will panic if c.pluginAPI is nil; add a
nil check for c.pluginAPI before calling Session.Get and return a typed error
(e.g., ErrPluginAPIUnavailable or a wrapped error) when it's nil, then only call
c.pluginAPI.Session.Get when c.pluginAPI is non-nil and proceed to validate
mmSession as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review: No Actionable Findings
Reviewed the full diff (3014 additions, 424 deletions across 38 files) for this MCP client catalog hardening PR. The changes are security-positive overall.
Key areas examined:
channelAnalysisToolBot tool expansion (api/api_channel.go): Overrides the bot's MCP tool allowlist for channel analysis. Initially appeared risky since channel analysis auto-approves all tool calls, but the downstream AnalyzeChannel function correctly scopes the tool set to only read_channel and get_channel_info with bound channel_id parameters before the LLM sees them. The expanded catalog is only used to locate these two tools.
Context propagation: context.Background() removed from production MCP tool paths. GetToolsForUser, createAndStoreUserClient, ConnectToRemoteServers, and ConnectToEmbeddedServerIfAvailable now accept caller-scoped context.Context. Tool resolvers require RequestContext and fail fast if missing.
Tool namespacing and bare-name fallback (llm/tools.go): lookupTool bare-name resolution correctly returns ambiguous results as not-found. mcpToolAllowed scopes allowlist checks by ServerOrigin, preventing cross-server tool confusion.
Concurrency safety: New mutex protection (toolsMu, clientsMu, discoveryMu, notifyOwnerMu) on Client and UserClients prevents race conditions during tool list invalidation and rediscovery.
Plugin MCP client (mcp/client.go): NewPluginClient correctly injects authenticated user ID via headerTransport from session-derived data, not user input. Tool list changed handlers properly invalidate caches.
Trace logging (llm/tools.go): TraceResolved logs full tool results when doTrace is true, but this flag is not enabled in any production code path.
No medium, high, or critical vulnerabilities identified.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review: No Actionable Findings
Reviewed the full diff for this MCP client catalog hardening PR. The changes are security-positive overall.
Key areas examined:
channelAnalysisToolBot tool expansion (api/api_channel.go): Overrides the bot's MCP tool allowlist (AutoEnableNewMCPTools = true, EnabledMCPTools = nil). Confirmed safe: channels/channels.go:95-111 immediately scopes to a new scopedTools store containing only read_channel and get_channel_info with bound channel_id parameters before the LLM executes.
lookupTool bare-name fallback (llm/tools.go): When the LLM calls a tool by bare name, the store searches all namespaced tools. Correctly returns not-found when ambiguous (multiple servers share the same bare tool name). The resolver always executes within the requesting user's permission scope.
ToolPolicyLookupName and policy bypass (mcp/tool_policy.go): The production ToolPolicyFunc wraps LookupToolPolicy, which calls ToolPolicyLookupName to strip namespace prefixes before matching admin-configured policies. Namespaced runtime names (jira__get_issue) correctly match bare policies (get_issue). No bypass path exists.
Context propagation: context.Background()/context.TODO() removed from MCP tool paths. Tool resolvers now require RequestContext and fail fast with "missing request context for MCP tool call" if nil. This is a security improvement.
Plugin MCP client (mcp/client.go:NewPluginClient): MMUserIDHeader is injected from c.userID (sourced from the authenticated session, not user input). The sourcePluginAPI nil check is preserved.
Concurrency safety: New mutex protection (toolsMu, clientsMu, discoveryMu) prevents race conditions during ToolListChanged notifications and rediscovery. createAndStoreUserClient uses a proper double-check-after-lock pattern.
Trace logging (llm/tools.go): TraceResolved logs full tool results, but only when doTrace is true. The production NewToolStore() call (at llmcontext/llm_context.go:191) passes no options, leaving doTrace as false.
No medium, high, or critical vulnerabilities identified.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review — No Findings
Reviewed the full diff (2840 additions across 36 files) for vulnerabilities in channel data isolation, user identity handling, tool/MCP boundaries, prompt injection surface, file access control, search scoping, and secrets exposure.
Security-positive changes in this PR:
Request context propagation: GetToolsForUser and tool resolvers now require context.Context from the authenticated request, replacing prior context.Background()/context.TODO() calls. This ensures MCP tool calls respect request-scoped cancellation and cannot outlive the originating session.
Tool namespacing: MCP tools are now namespaced by server slug (e.g., jira__search instead of search), eliminating cross-server tool name collisions. The lookupTool bare-name fallback correctly rejects ambiguous matches.
Concurrency safety: Client.tools access is now mutex-protected, and UserClients operations use proper RLock/Lock patterns with snapshot-then-release to avoid holding locks during network calls.
Tool resolver hardening: The resolver now fails fast with "missing request context for MCP tool call" when RequestContext is nil, enforcing the coding guideline against context.Background() in production paths.
Request-scoped error handling: Plugin connection errors are no longer persisted on the cached UserClients, preventing transient failures from leaking across requests.
Areas examined with no issues found:
channelAnalysisToolBot: Overrides the bot's AutoEnableNewMCPTools/EnabledMCPTools to load the full MCP catalog. While this expands available tools beyond the bot's configured allowlist, the requesting user's identity and permissions still scope all tool execution via RequestContext. The admin-level filterToolsByConfig disabled-tool filtering still applies. This is a policy choice, not a permission bypass.
NewPluginClient: User identity is correctly baked into the HTTP transport via MMUserIDHeader from the session-authenticated userID, not from user-supplied input.
ToolPolicyLookupName / mcpToolAllowed bare-name fallbacks: Always scoped by ServerOrigin, so cross-server policy confusion is not possible.
No hardcoded credentials, secrets in logs, or path traversal vectors introduced.
The reason will be displayed to describe this comment to others. Learn more.
I cleaned this up by moving the dynamic MCP/tool execution fields into a nested ToolRuntimeContext on llm.Context, so they’re no longer loose prompt-building fields.
I think this should stay reachable from llm.Context because Context is already the per-turn carrier used by tool construction and execution (Tools already lives there), not just rendered prompt text. The dynamic MCP state needs to be available across the same flow: building the tool store, resolving meta-tools, recording per-turn load/search telemetry, and restoring loaded tools. Splitting it into a separate top-level parameter would force the same runtime object through all resolver/toolrunner/context-builder paths without changing the ownership model. If you're passing it all around to the same places alongside the Context it's looking like a shared tool context anyways.
The reason will be displayed to describe this comment to others. Learn more.
Remote servers: require/configure a name through system console; serverID is that name anyway.
Plugin MCP with a configured name: client.config.Name is the plugin-provided name, so that wins.
Plugin MCP without a configured name: client.config.Name is empty, then fallback uses serverID, which is plugin://.
For that fallback case, serverID and BaseURL are basically the same synthetic origin, so there isn't much of a difference in ordering. I preserved both fallbacks more for safety than anything.
The reason will be displayed to describe this comment to others. Learn more.
unloadedMCPTools are tools we know to exist in the catalog (ie, from a search) but haven't been loaded yet. ToolInfo is a lightweight struct to reflect what we actually know about it at this point. I've added a comment to clarify
The reason will be displayed to describe this comment to others. Learn more.
Is this for supporting the MCP feature where an MCP server can add tools while holding the connection open? This feels orthogonal to what this PR is trying to accomplish. Maybe a separate PR? Not sure it's even worth supporting.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security Review — No Findings
Reviewed the full diff (2564 additions, 474 deletions across 43 files) for vulnerabilities in channel data isolation, user identity handling, tool/MCP boundaries, prompt injection surface, file access control, search scoping, and secrets exposure. CI checks pass (31/32 passed, 1 skipped).
Security-positive changes in this PR:
Request context propagation: GetToolsForUser, ConnectToRemoteServers, ConnectToEmbeddedServerIfAvailable, and tool resolvers now accept context.Context from the authenticated request, replacing context.Background()/context.TODO() calls. MCP tool calls now respect request-scoped cancellation.
Tool namespacing: MCP tools are namespaced by server slug (e.g., jira__search), preventing cross-server tool name collisions. lookupTool bare-name fallback correctly rejects ambiguous matches (multiple servers sharing the same bare name).
Concurrency safety: Client.tools and UserClients.clients access is now mutex-protected with proper RLock/Lock patterns. Tools() returns maps.Clone() to prevent concurrent modification.
Request-scoped error handling: Plugin connection errors are no longer persisted on the cached UserClients (cloneMCPErrors + local appendMCPError), preventing transient failures from leaking across requests.
Areas examined with no issues found:
channelAnalysisToolBot (api/api_channel.go:179): Overrides the bot's AutoEnableNewMCPTools/EnabledMCPTools to load the full MCP catalog for tool discovery. Confirmed safe: channels/channels.go:96-111 immediately scopes to a new scopedTools store containing only read_channel and get_channel_info with bound channel_id parameters before the LLM executes. No other tools reach the ToolRunner.
NewPluginClient (mcp/client.go): User identity is injected via MMUserIDHeader from the session-authenticated userID (not user-supplied input). sourcePluginAPI nil check is preserved.
ToolPolicyLookupName / mcpToolAllowed: Bare-name fallbacks are always scoped by ServerOrigin, preventing cross-server policy confusion. LookupToolPolicy dispatches by serverBaseURL before stripping namespaces.
lookupTool / lookupUnloadedMCPTool: Both iterate all tools for bare-name matches but return not-found on ambiguity (multiple matches), preventing unintended tool resolution.
No hardcoded credentials, secrets in logs/responses, path traversal vectors, or SSRF vectors introduced.
Removed upstream in PR #542 (5ab61bf) when OpenTelemetry replaced
log-based tracing. Restore master's no-arg NewToolStore signature
and drop TraceLog, TraceUnknown, TraceResolved, and
LogUnknownToolWarning while keeping the dynamic-MCP additions.
The reason will be displayed to describe this comment to others. Learn more.
Security Review — No Findings
Reviewed the full diff (2564 additions, 474 deletions across 46 files) for vulnerabilities in channel data isolation, user identity handling, tool/MCP boundaries, prompt injection surface, file access control, search scoping, and secrets exposure. CI checks pass (33/34, 1 skipped).
Security-positive changes in this PR:
Request context propagation: GetToolsForUser, ConnectToRemoteServers, ConnectToEmbeddedServerIfAvailable, and tool resolvers now accept context.Context from the authenticated request. The ToolResolver signature itself was updated to func(ctx context.Context, llmCtx *Context, argsGetter ToolArgumentGetter), eliminating context.Background() shortcuts across production tool-call paths. MCP tool calls now respect request-scoped cancellation and timeouts.
Concurrency safety: Client.tools access is now protected by toolsMu (RWMutex), and UserClients.clients by clientsMu. Tools() returns maps.Clone() to prevent concurrent map modification. ConnectToEmbeddedServerIfAvailable and ConnectToPluginServer use double-check-after-lock patterns to avoid duplicate connections.
Request-scoped error handling: cloneMCPErrors + local appendMCPError ensure plugin connection errors are request-scoped and no longer persisted on the cached UserClients, preventing transient failures from leaking across requests.
Tool namespacing: MCP tools are namespaced by server slug (e.g., jira__search), preventing cross-server tool name collisions. lookupTool bare-name fallback correctly rejects ambiguous matches (multiple servers sharing the same bare name return not-found).
Areas examined with no issues found:
channelAnalysisToolBot (api/api_channel.go:178): Creates a modified bot with AutoEnableNewMCPTools=true and EnabledMCPTools=nil to expand MCP catalog discovery. Confirmed safe: channels/channels.go:96-111 immediately replaces the tool store with a scopedTools store containing only read_channel and get_channel_info with bound channel_id parameters before the LLM executes. No other tools reach the ToolRunner.
lookupTool bare-name fallback (llm/tools.go:360-383): When the LLM calls a tool by bare name, the store searches all namespaced tools. Correctly returns not-found when ambiguous (multiple MCP servers share the same bare tool name). Built-in tools (empty ServerOrigin) are excluded from bare-name matching. The resolver always executes within the requesting user's permission scope.
ToolPolicyLookupName / LookupToolPolicy (mcp/tool_policy.go): Runtime namespaced names (e.g., jira__get_issue) are correctly stripped to bare names before matching admin-configured policies. An exact configured name still wins over bare-name fallback. Policy lookups are always scoped by serverBaseURL, preventing cross-server policy confusion.
shouldAutoExecuteTool / botChannelAutoEverywhereKeepTool: Both pass namespaced runtime names through the ToolPolicyChecker, which dispatches to LookupToolPolicy → ToolPolicyLookupName. Namespace stripping ensures namespaced tools correctly match their configured auto-run/auto-run-everywhere policies.
NewPluginClient (mcp/client.go:311-388): User identity is injected via MMUserIDHeader from userID (sourced from the authenticated session, not user-supplied input). sourcePluginAPI nil check is the first guard.
ToolRuntimeContext (llm/context.go): New runtime fields (KeepMCPTool, PreloadedMCPTools, restoreMCPDynamicTools, telemetry hooks) are scaffolding for dynamic loading in later stack PRs. All fields are set internally during context building, never from user input.
No hardcoded credentials, secrets in logs/responses, path traversal vectors, or SSRF vectors introduced.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
nickmisasi
force-pushed
the
dynamic-mcp-02-client-catalog
branch
from
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
crspeller
left a comment
Summary
Hardens MCP client catalog discovery with safer tool identity handling (duplicate registrations are no longer silently dropped), plugin/embedded discovery refresh behavior, user-client synchronization, and retrieval override plumbing, with backwards-compatible tool policy lookup for configs without namespace plumbing. Also adds bare MCP tool name resolution, LLM context scaffolding for dynamic loading (request context, registry stash, telemetry hooks, preloaded tools), threads request context through conversation and API entry points, fixes channel analysis tool discoverability under narrow allowlists, and removes legacy log-based tool-trace plumbing superseded by OpenTelemetry.
Stack: 2/6
Ticket Link
N/A
Screenshots
N/A
Release Note
Made with Cursor
Summary by CodeRabbit
New Features
Improvements
Tests